Skip to content

Conversation

@toupper
Copy link
Contributor

@toupper toupper commented Oct 30, 2025

WOOMOB-1155

Description

Analytics events: pdfdoF-7XF-p2

  • orders_menu_item_tapped - Orders menu item tapped (no properties)
  • orders_list_loaded - Orders view loaded (no properties)
  • orders_list_pull_to_refresh - Pull to refresh triggered (no properties)
  • orders_list_fetched - Orders fetched completion with milliseconds_since_request_sent
  • orders_list_next_page_loaded - Next page loaded with page_number
  • orders_list_row_tapped - Order row tapped with order_id, order_status, list_position (0 for topmost), days_since_created (0 for today, 1 for yesterday)
  • orders_list_search_button_tapped - Search button tapped (no properties)
  • orders_list_search_results_fetched - Search results fetched completion with milliseconds_since_request_sent
  • order_details_loaded - Order details loaded with order_id, order_status, days_since_created (0 for today, 1 for yesterday)
  • order_details_email_receipt_tapped - Email receipt tapped (no properties)

Events registering are coming.

Steps to reproduce

  1. Launch the app and POS
  2. Select Menu -> Orders
  3. Navigate the flow in various ways to confirm the behavior:
    • View orders list
    • Pull to refresh
    • Paginate
    • Search for orders and paginate
    • Tap on order rows
    • View order details
    • Tap the email receipt button

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@toupper toupper marked this pull request as draft October 30, 2025 09:42
@dangermattic
Copy link
Collaborator

dangermattic commented Oct 30, 2025

1 Warning
⚠️ This PR is assigned to the milestone 23.6. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.
1 Message
📖

This PR contains changes to Tracks-related logic. Please ensure (author and reviewer) the following are completed:

  • The tracks events must be validated in the Tracks system.
  • Verify the internal Tracks spreadsheet has also been updated.
  • Please consider registering any new events.
  • The PR must be assigned the category: tracks label.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 30, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit70de4e3
Direct Downloadwoocommerce-wear-prototype-build-pr14863-70de4e3.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 30, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit70de4e3
Direct Downloadwoocommerce-prototype-build-pr14863-70de4e3.apk

@toupper toupper changed the title Feat/woomob 1155 pos orders analytics [Woo POS][Historical Orders] Order Details: Analytics Oct 30, 2025
@toupper toupper added feature: analytics In-app store analytics feature: POS labels Oct 30, 2025
@toupper toupper added this to the 23.6 milestone Oct 30, 2025
@toupper toupper marked this pull request as ready for review October 30, 2025 15:02
@toupper toupper requested a review from kidinov October 30, 2025 15:02
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 41.86047% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.26%. Comparing base (c197fbe) to head (70de4e3).
⚠️ Report is 4 commits behind head on trunk.

Files with missing lines Patch % Lines
...d/ui/woopos/util/analytics/WooPosAnalyticsEvent.kt 4.00% 48 Missing ⚠️
.../android/ui/woopos/orders/WooPosOrdersViewModel.kt 93.75% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #14863   +/-   ##
=========================================
  Coverage     38.26%   38.26%           
- Complexity    10085    10087    +2     
=========================================
  Files          2131     2131           
  Lines        120708   120793   +85     
  Branches      16537    16541    +4     
=========================================
+ Hits          46187    46225   +38     
- Misses        69825    69870   +45     
- Partials       4696     4698    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @toupper ! Overal looks good to me, but I'd propose a few changes with the main goal to keep it simplier

Implement_WooPosOrdersAnalyticsTracker_for_enhanced_order_analytics_tracking.patch

Proposed Changes: Extract Tracking Logic from WooPosOrdersViewModel

  1. Add tracking fields to OrderItemViewState, to avoid separate "caching"
  // WooPosOrdersState.kt
  data class OrderItemViewState(
      ...
      val statusSlug: String,
      val createdAtMillis: Long
  )
  1. Create WooPosOrdersAnalyticsTracker class

New file with all tracking methods as suspend functions:

  • trackOrdersListFetched(elapsedMs)
  • trackOrdersListRowTapped(orderId, orderStatus, listPosition, createdAtMillis)
  • trackOrderDetailsLoaded(orderId, orderStatus, createdAtMillis)
  • etc.
  1. Remove from ViewModel:
  • OrderTrackingMeta data class
  • trackingMeta mutable map
  • cacheOrderTrackingMeta() method
  • retrieveOrderTrackingData() helper
  • trackOrderDetailsShown() and trackOrdersListRowTapped() private methods
  • 9 analytics event imports + MILLIS_PER_DAY
  1. Update ViewModel:
  • Inject WooPosOrdersAnalyticsTracker instead of WooPosAnalyticsTracker
  • Replace all direct analyticsTracker.track() calls with ordersAnalyticsTracker.track*() in viewModelScope.launch
  • Track OrderDetailsLoaded directly in onOrderSelected() (no separate UI callback needed)

Why This Is Better:

  • Separation of concerns: Tracking logic separated from state management
  • No mutable cache: Tracking data lives naturally in the view state
  • Cleaner: Removes ~60 lines from ViewModel
  • Testable: Analytics logic can be tested in isolation
  • Maintainable: Changes to tracking don't touch ViewModel and don't make it even bigger

Wdyt?

@toupper
Copy link
Contributor Author

toupper commented Oct 31, 2025

Thanks for the patch @kidinov! I added it here a8dec03

One question — as in the patch, I initially added the tracking data into the state, but after some research, I found that it’s generally better to keep only what’s strictly needed for the UI there. In this case, memory usage would be the same since we store it in another structure, but keeping it out of the state avoids unnecessary recompositions and keeps the state cleaner and more focused on UI data. What do you think — do we have a convention for handling this kind of case?

@toupper toupper added category: tracks Related to analytics, including Tracks Events. unit-tests-exemption labels Oct 31, 2025
@toupper toupper closed this Oct 31, 2025
@toupper toupper reopened this Oct 31, 2025
@toupper
Copy link
Contributor Author

toupper commented Oct 31, 2025

@kidinov Also about this one

Track OrderDetailsLoaded directly in onOrderSelected() (no separate UI callback needed)

If we couple the OrderDetailsLoaded event to onOrderSelected, we’ll miss the initial auto-selected case (first item on load/refresh), and we’re not matching the event’s intent. Selection is an intention; loading details is an outcome. A bug could select an order without actually rendering/loading its details. I know that second case shouldn’t happen with our current flow, but it’s still a leaky proxy. What do you think?

@toupper toupper requested a review from kidinov October 31, 2025 08:11
Copy link
Contributor

@kidinov kidinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@toupper

In this case, memory usage would be the same since we store it in another structure, but keeping it out of the state avoids unnecessary recompositions and keeps the state cleaner and more focused on UI data. What do you think — do we have a convention for handling this kind of case?

That's all correct, except in this case you do the changes only when the state that changes the UI changes as well, so no extra recompositions unless I am missing something. All this should be handled by having all the orders we show in cache, and then we would take the order data lazily, when it's needed to be tracked, from the cache. I hope we'll implement this in I2 of this project alongside the replacement of the refetching of the order after email sent and/or refund made.

If we couple the OrderDetailsLoaded event to onOrderSelected, we’ll miss the initial auto-selected case (first item on load/refresh), and we’re not matching the event’s intent. Selection is an intention; loading details is an outcome. A bug could select an order without actually rendering/loading its details. I know that second case shouldn’t happen with our current flow, but it’s still a leaky proxy. What do you think?

Yep, you are right. My intention was to simplify it by not passing it via the UI layer, but indeed it's not ideal. The workaround, which I don't like but avoids having this round trip, is here. So apparently no ideal solution here - up to you
Track_order_details_loading_in_WooPosOrdersViewModel.patch

Also, I noticed that we now track pos_orders_list_fetched and pos_orders_list_search_results_fetched on success, but as peer description it's about request completion - check the patch out

Track_elapsed_time_for_orders_list_search_and_fetch_operations.patch

Other than that, LGTM!

@toupper toupper merged commit 3c11cd3 into trunk Oct 31, 2025
18 checks passed
@toupper toupper deleted the feat/WOOMOB-1155-pos-orders-analytics branch October 31, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: tracks Related to analytics, including Tracks Events. feature: analytics In-app store analytics feature: POS unit-tests-exemption

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants